-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Sync Rules #109
Implement Sync Rules #109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works nicely! Some comments:
src/components/rules-creation-page/steps/InstanceSelectionStep.jsx
Outdated
Show resolved
Hide resolved
src/components/rules-creation-page/steps/InstanceSelectionStep.jsx
Outdated
Show resolved
Hide resolved
name: this.props.d2.models["dataElement"].displayName, | ||
id: this.props.d2.models["dataElement"].name, | ||
}, | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the structure name/id won't change, we could do it programmatically in a loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the idea was to keep the same structure in the app and DB So let's do this only if it really makes sense to reduce the size of the persisted object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do in another PR, we might extract the models altogether and pass them as a prop
src/components/rules-creation-page/steps/MetadataSelectionStep.jsx
Outdated
Show resolved
Hide resolved
...this.syncRule.builder, | ||
targetInstances, | ||
}, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! I know at the beginning it feels cumbersome, but in my experience it pays off, on the long run, to have fully immutable models. By the way, this kind of boilerplate for nested updates can be simplified using lodash.fp, (_.set), but please don't change it, the method is not fully typed and we lose safety. There is a functional pattern to work with nested structures called "lenses". For TS, this is the best I know: https://github.com/utatti/lens.ts. Again, don't change it, just as a note, it's perfectly fine as it is, with just a level of nesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIll take a look on this lenses concept!
First of all, let me say that this incredible work! Thanks for this. A few minor comments (some of them already discussed):
I am merging this anyway as it is a huge PR and I don't want to postpone it anymore. Feel free to create issues if you agree with my comments or let me know otherwise! |
📌 References
📝 Implementation
Add two new routes to access the new pages
Add Sync Rules Landing Page
Add Sync Rules Creation Wizard
Add General Info Step with a simple form
Add Metadata Selection Step with ObjectsTable that allows any type of metadata
Add Instance Selection Step with a multiselector
Add Save Step with a Summary of the Sync Rule to save in the dataStore
Update dropdown to allow hiding default value and fixing bug where it goes off-screen
Move getValueForCollection method to utils and re-use it in the new components
Fix bug in datastore that crashed when searching in non-strings
Add a new list by id logic method that does not require knowing the metadata type
Allow setting the fields when accessing the metadata endpoint
🎨 Screenshots
🔥 Is there anything the reviewer should know to test it?
📑 Others